#68 OpenApi Endpoint Properties & Missing ChatEndpoint#69
#68 OpenApi Endpoint Properties & Missing ChatEndpoint#69pandapknaepel wants to merge 4 commits intoOkGoDoIt:masterfrom
Conversation
by usage of the interfaces of the endpoints
by the ChatEndpoint
fixed auth tests
|
closes #68 |
pandapknaepel
changed the title
Baklap4
left a comment
Baklap4
left a comment
There was a problem hiding this comment.
I'm just gonna comment as i'm a random guy on the internet and I am in no way able to approve/request changes since i'm not the owner of the codebase. But in a mainainability standpoint some things should be refactored i think.
| /// Represents authentication to the OpenAPI API endpoint | ||
| /// </summary> | ||
| public class APIAuthentication | ||
| public class APIAuthentication : IAPIAuthentication |
There was a problem hiding this comment.
If APIAuthentication is not meant to be inherited please add the keyword sealed so it's explicitly defined. This comes with some performance benefits: see this PR from the dotnet runtime: dotnet/runtime#49944
| /// <summary> | ||
| /// Represents authentication to the OpenAPI API endpoint | ||
| /// </summary> | ||
| public interface IAPIAuthentication |
There was a problem hiding this comment.
In my opinion POCO's (Plain old C# objects) shouldn't have an Interface. However this isn't a poco as it's having a method within it to validate the key.
Personally i'd extract the Validate logic to a service which can validate stuff (for now the api-key).
If you want this APIAuthentication to be available through DI, one should look into using the Options Pattern as this is meant to fix that problem.
| /// Tests the api key against the OpenAI API, to ensure it is valid. This hits the models endpoint so should not be charged for usage. | ||
| /// </summary> | ||
| /// <returns><see langword="true"/> if the api key is valid, or <see langword="false"/> if empty or not accepted by the OpenAI API.</returns> | ||
| Task<bool> ValidateAPIKey(); |
There was a problem hiding this comment.
The implementation of this method does a couple of things. Maybe extract that logic to definitive classes which each do their own thing?
Also this method should end in Async since it's an asynchronous operation.
I'd change the name of the method to: IsApiKeyValidAsync(); so it reads nicely in if/switch-statements
2 participants
Here's a quick rundown of what I've done:
I'm confident that these changes will help to enhance the overall quality and maintainability of the codebase. Please let me know if you have any questions or feedback on these changes.